Skip to content

fix(backup): wait for the backup slot on the update path, and stop blaming the backup - #90742

Closed
isndotbiz wants to merge 1 commit into
NousResearch:mainfrom
isndotbiz:fix/pre-update-backup-lock-timeout
Closed

isndotbiz wants to merge 1 commit into
NousResearch:mainfrom
isndotbiz:fix/pre-update-backup-lock-timeout

Conversation

@isndotbiz

Copy link
Copy Markdown

What does this PR do?

hermes update --backup can print

◆ Creating pre-update backup...
  ⚠ Backup skipped (no files found or write failed); continuing update.

and then run the whole update with no rollback point — on an install where the backup works
perfectly. Neither half of that message is true.

_backup_operation_lock (added in #77913 to serialize snapshots) waits 0.25s for the shared
backup slot. On timeout it raises BackupInProgressError, which _write_full_zip_backup swallows
into a logger.warning and a None return — the same None the caller gets from an empty scan or
a failed write. _run_pre_update_backup therefore cannot tell contention from failure, and reports
a lost quarter-second race as a broken backup.

Two things are wrong with that, and this PR fixes both:

  1. 0.25s is the wrong wait for the update path. A fail-fast makes sense for an interactive
    hermes backup — the user is sitting there and can retry. The pre-update backup is the only
    thing standing between a bad update and an unrecoverable ~/.hermes (cf. hermes update --yes wiped entire ~/.hermes/ directory (.env, MEMORY.md, kanban.db, skills, scripts — all gone) #48200), so it should
    wait rather than skip. Any concurrent snapshot — a scheduled backup, the desktop/gateway
    process, a second terminal — is enough to lose the race.
  2. The message sends you after the wrong bug. It cost me an investigation: I went looking for a
    broken backup, and found a backup that works.

Related Issue

No existing issue — found while debugging a live install. Adjacent, different causes:
#75724 (a non-SQLite .db aborts the full backup) and #48200 (the update that wiped ~/.hermes,
which is why the pre-update backup exists at all).

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

hermes_cli/backup.py

  • _BACKUP_LOCK_DEFAULT_TIMEOUT = 0.25 — names the existing interactive default; unchanged.
  • _PRE_UPDATE_LOCK_TIMEOUT = 180.0 — the update path's wait. Bounded so a wedged backup process
    cannot stall an update forever.
  • _backup_operation_lock logs one "waiting for the Hermes backup slot" warning after 2s, so a
    multi-minute wait does not look like a hang.
  • _write_full_zip_backup(..., lock_timeout=..., raise_if_busy=False) — raise_if_busy re-raises
    BackupInProgressError instead of collapsing it into None, so a caller can say which happened.
  • create_pre_update_backup(..., lock_timeout=_PRE_UPDATE_LOCK_TIMEOUT, raise_if_busy=False) and
    create_quick_snapshot(..., lock_timeout=_BACKUP_LOCK_DEFAULT_TIMEOUT) plumb it through.

hermes_cli/update_cmd.py

  • The quick snapshot and the full zip both wait _PRE_UPDATE_LOCK_TIMEOUT on the update path,
    passed explicitly at both call sites. Not left to the default argument: a default binds at def
    time, so a timeout taken that way cannot be overridden and cannot be tested without sitting
    through the real wait (which is how my own test first passed for the wrong reason — see below).
  • Contention now prints what actually happened, and says the backup itself is fine.
  • The remaining None case says "nothing to archive, or the archive could not be written" instead
    of asserting both at once.

Backwards compatibility: raise_if_busy defaults to False and the lock's default timeout is
unchanged, so hermes backup and create_pre_migration_backup behave exactly as before.

How to Test

Reproduction (no patch): hold the slot in one process, ask for a backup in another.

import time
from hermes_cli import backup as B
from hermes_cli.config import get_hermes_home

with B._backup_operation_lock(get_hermes_home()):
    print(B.create_pre_update_backup(keep=5))   # -> None, in ~0.25s

Before: None after 0.25s, and hermes update --backup prints "no files found or write failed".
After: the call waits for the slot; if it never frees within the bound, hermes update says the
slot was held and that the backup is not the problem.

That the backup is healthy the whole time — same install, slot free:

create_pre_update_backup() -> ~/.hermes/backups/pre-update-....zip
14,003 files, 345 MB, 80s, every .db through _safe_copy_db

Tests added in tests/hermes_cli/test_backup_stability.py:

  • test_busy_slot_is_distinguishable_from_a_failed_write
  • test_pre_update_backup_reports_a_busy_slot_when_asked
  • test_pre_update_backup_waits_for_the_slot

and in tests/hermes_cli/test_backup.py:

  • TestRunPreUpdateBackup::test_full_mode_names_lock_contention_instead_of_blaming_the_backup
    (asserts the old "no files found" wording is gone, and that the patched timeout actually reaches
    the call — it caught a real defect: the first version of this patch took the timeout as a default
    argument, so the test waited out the full 180s and passed for the wrong reason. The full-suite
    run surfaced it as a 120s pytest-timeout; the assertion now fails fast instead.)

Checklist

Code

On the suite: I ran all of tests/hermes_cli (634 files) under the install's own venv:
5,922 passed, 63 skipped, 9 errors in 29m40s. All 9 errors are teardown errors in
tests/hermes_cli/test_web_server_approvals_broadcast.py
(AttributeError: 'types.SimpleNamespace' object has no attribute '_methods' in the
_reset_tui_gateway_server_state fixture) — nothing to do with this change; that install carries a
local modification to hermes_cli/web_server.py, so I would not read those as upstream-clean
either. The one failure that was mine — the pre-update backup test hitting a 120s timeout — is
the defect described above and is fixed in this PR; the affected suites now run
97 passed in 1m40s (test_backup.py, test_backup_stability.py, test_cmd_update.py).

I did not run the other ~2,400 test files outside tests/hermes_cli, so I have not checked that
box.

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings on the changed functions) — the new constants
    are internal, so no cli-config.yaml.example key was added
  • N/A — no config keys added
  • N/A — no architecture or workflow change
  • Cross-platform: the change is in the shared timeout/plumbing path, above the
    msvcrt/fcntl split; both branches get the same _notice() and the same deadline
    arithmetic. Not executed on Windows or macOS.
  • N/A — no tool behaviour change

…aming the backup

`hermes update --backup` could print

    ⚠ Backup skipped (no files found or write failed); continuing update.

and carry on with no rollback point. Neither half of that message was true.

`_backup_operation_lock` waits 0.25s for the shared backup slot. On timeout it
raises BackupInProgressError, which `_write_full_zip_backup` swallowed into a
logger.warning and a None return — indistinguishable, at the call site, from an
empty scan or a failed write. So losing a quarter-second race with any
concurrent snapshot silently skipped the only backup an update takes.

Reproduced on a live install: create_pre_update_backup() called directly wrote
14,003 files / 345 MB in 80s with every .db passing safe-copy, while the same
call under a held lock returned None in exactly 0.25s.

- The update path now waits up to _PRE_UPDATE_LOCK_TIMEOUT (180s) for the slot,
  for both the quick snapshot and the full zip. An update that waits beats an
  update that proceeds unprotected; the bound keeps a wedged backup from
  stalling it forever. The interactive default stays 0.25s — `hermes backup`
  should still fail fast.
- One "waiting for the backup slot" warning after 2s, so a multi-minute wait
  does not look like a hang.
- raise_if_busy lets the caller tell contention from failure. `hermes update`
  now names the lock conflict and says the backup itself is fine; the remaining
  None case says "nothing to archive, or the archive could not be written"
  instead of asserting both.

Existing callers are untouched: raise_if_busy defaults to False, so
create_pre_migration_backup and `hermes backup` keep the old swallow-and-None
behaviour and the old timeout.
Copilot AI lite review requested due to automatic review settings August 20, 2026 10:37
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 20, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a misleading and unsafe behavior in the hermes update --backup flow where a brief lock-contention race could be reported as a “skipped/failed” backup and allow the update to proceed without a rollback artifact, even though the backup system itself was healthy.

Changes:

  • Introduces distinct lock-timeout policies for interactive backups vs. pre-update backups, and adds a one-time “still waiting” notice while contending for the backup slot.
  • Makes backup-slot contention distinguishable from “no files / write failed” by optionally re-raising BackupInProgressError instead of collapsing it into a None result.
  • Updates hermes update messaging and adds regression tests to ensure contention is reported accurately and the intended timeout is actually plumbed through.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
hermes_cli/backup.py Adds separate timeouts for update vs. interactive paths; improves lock acquisition behavior; adds raise_if_busy/lock_timeout plumbing to distinguish contention from other None outcomes.
hermes_cli/update_cmd.py Passes the pre-update timeout explicitly to both snapshot and full-zip backups; prints accurate user messaging on lock contention vs. other skip cases.
tests/hermes_cli/test_backup.py Adds a regression test ensuring hermes update --backup reports lock contention (and that patched timeouts actually reach the call sites).
tests/hermes_cli/test_backup_stability.py Adds tests covering the new “busy slot vs. failed write” distinction and verifying the pre-update path uses the longer lock timeout.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@isndotbiz

Copy link
Copy Markdown
Author

Closing this as stale: it now conflicts with main and the underlying code has moved a long way since 2026-08-20. The report itself still stands if anyone wants to pick it up — _backup_operation_lock waits 0.25s for the shared backup slot, and on timeout _write_full_zip_backup swallows BackupInProgressError into a warning plus a None return, which the update path then reports as "Backup skipped (no files found or write failed)" and proceeds with no rollback point. Happy to reopen against current main if it's wanted.

@isndotbiz isndotbiz closed this Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants